Fix cache check by ratmice · Pull Request #525 · softdevteam/grmtools

ratmice · 2025-03-16T13:58:05Z

I think this should run the cache check,

E.g. at least the following subset of .buildbot.sh now caught a bug where the pretty printer
was emitting a non-whitespace change, and we had to add a comma.

#! /bin/sh

set -e
export RUSTFLAGS="--cfg grmtools_extra_checks"
root=`pwd`
cd $root/lrlex/examples/calc_manual_lex
echo "2 + 3 * 4" | cargo run | grep "Result: 14"
# Touching these files shouldn't invalidate the cache (via --cfg grmtools_extra_checks)
touch src/main.rs && CACHE_EXPECTED=y cargo build
cd $root/lrpar/examples/calc_actions
echo -n "2 + 3 * 4" | cargo run --package nimbleparse -- src/calc.l src/calc.y -
echo "2 + 3 * 4" | cargo run | grep "Result: 14"
touch src/main.rs && CACHE_EXPECTED=y cargo build
cd $root/lrpar/examples/calc_ast
echo -n "2 + 3 * 4" | cargo run --package nimbleparse -- src/calc.l src/calc.y -
echo "2 + 3 * 4" | cargo run | grep "Result: 14"
touch src/main.rs && CACHE_EXPECTED=y cargo build
cd $root/lrpar/examples/calc_parsetree
echo -n "2 + 3 * 4" | cargo run --package nimbleparse -- src/calc.l src/calc.y -
echo "2 + 3 * 4" | cargo run | grep "Result: 14"
touch src/main.rs && CACHE_EXPECTED=y cargo build
cd $root/lrpar/examples/clone_param
echo -n "1+++" | cargo run --package nimbleparse -- src/param.l src/param.y -
cd $root/lrpar/examples/start_states
echo -n "/* /* commented out */ */ uncommented text /* */" | cargo run --package nimbleparse -- src/comment.l src/comment.y -
cd $root

ratmice · 2025-03-16T13:58:59Z

With these changes, I think that #513 should consistently fail.

ratmice · 2025-03-16T14:05:47Z

cfgrammar/src/lib/yacc/mod.rs

            YaccKind::Grmtools => quote!(::cfgrammar::yacc::YaccKind::Grmtools),
            YaccKind::Original(action_kind) => {
-                quote!(::cfgrammar::yacc::YaccKind::Original(#action_kind))
+                quote!(::cfgrammar::yacc::YaccKind::Original(#action_kind,))


I think perhaps it is best to attempt to do something less fragile here.

My thoughts are instead of emitting the cache module,
emit a constant string in the hopes that the pretty printer will ignore the string.

E.g. so go from // CACHE INFORMATION ... to const CACHE_INFORMATION: &str = "..."
abandoning cache_module unless we do want to keep it?

Fixed by 8e32c81 and subsequent commits...

ratmice · 2025-03-16T14:34:18Z

Might want to sqaush out the a08fbdb change?

ratmice · 2025-03-16T14:50:12Z

lrpar/src/lib/ctbuilder.rs

-        quote! {
+        let cache_info = quote! {
            #[allow(unused)]
            mod _cache_information_ {


We may want to do something else here, as we no longer actually compile this code
as we change it it could perhaps start to contain syntax errors, etc.

We could emit both the cache_info module, and the module text within that constant string.
Or we could just emit a formatted string more like the original comment.

Don't we just want to output the information in one place? Perhaps I've misunderstood!

Yeah, I guess i'm saying:

this emits a strong:

const CACHE_INFO: &str = "#[allow(unused)] mod _cache_information_ {...

But because we never actually compile mod _cache_information_ anymore, it is just embedded in that string,
so it is both confusing, and we could make a change that would cause mod _cache_information_ to not actually compile but not notice.

I basically just did the minimal change to check that embedding it as a string works as a proof of concept.

Should be fixed in a687505

ltratt · 2025-03-16T14:57:14Z

Shouldn't we remove mod _cache_information then?

ratmice · 2025-03-16T15:12:21Z

lrpar/src/lib/ctbuilder.rs

+            // This declaration can be affected by the pretty printer.
+            // But we would hope the actual cache string is not.
+            #[allow(unused)]
+            const CACHE_INFO: &str = #cache;


Looks like we can name this const _: &str = #cache.

I thought this wasn't a valid constant declaration, but it looks like it is now.

Edit it was allowed in: RFC 2562

Also allows us to get rid of #[allow(unused)]

Fixed by 3b860ca

ratmice · 2025-03-16T15:28:02Z

So, I think this is much less fragile now than trying to rely on the whitespace hack.
Probably ready for review.

ltratt · 2025-03-16T18:02:28Z

Please squash.

This ran into a known cargo footgun where if you emit rerun-if-changed you are responsible for emitting all the rerun-if-changed directives that otherwise would be implied by cargo.

The cache check only runs if the output grammar is newer.

ratmice · 2025-03-16T18:17:17Z

Squashed.

ratmice · 2025-03-16T18:27:01Z

Sorry, I really need to figure out how to configure my editor so it can conditionally automatically run rustfmt or not depending upon which directory I'm editing.

Because I have some projects that don't use it.

Will need a squash

ltratt · 2025-03-16T21:26:23Z

Please squash.

This should make the cache more robust to changes that are caused by the pretty printer.

ratmice · 2025-03-16T21:40:43Z

Squashed.

ratmice assigned ratmice and ltratt Mar 16, 2025

ratmice commented Mar 16, 2025

View reviewed changes

ratmice added 3 commits March 16, 2025 11:14

Revert ac1bf46 which ends up disabling rerun-if-changed for src/*.rs

04796af

This ran into a known cargo footgun where if you emit rerun-if-changed you are responsible for emitting all the rerun-if-changed directives that otherwise would be implied by cargo.

When testing cache check don't modify the input grammar.

93450de

The cache check only runs if the output grammar is newer.

Simplify buildbot cache check.

13e3a05

ratmice force-pushed the fix_cache_check branch from 3b860ca to 5565880 Compare March 16, 2025 18:15

ltratt added this pull request to the merge queue Mar 16, 2025

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 16, 2025

Replace _cache_info_ module with a constant string.

93028a5

This should make the cache more robust to changes that are caused by the pretty printer.

ratmice force-pushed the fix_cache_check branch from 24b77a2 to 93028a5 Compare March 16, 2025 21:40

ltratt added this pull request to the merge queue Mar 16, 2025

Merged via the queue into softdevteam:master with commit 6ec4291 Mar 16, 2025
2 checks passed

ratmice deleted the fix_cache_check branch March 16, 2025 22:08

Conversation

ratmice commented Mar 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ratmice commented Mar 16, 2025

Uh oh!

ratmice Mar 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ratmice Mar 16, 2025

Choose a reason for hiding this comment

Uh oh!

ratmice commented Mar 16, 2025

Uh oh!

ratmice Mar 16, 2025

Choose a reason for hiding this comment

Uh oh!

ltratt Mar 16, 2025

Choose a reason for hiding this comment

Uh oh!

ratmice Mar 16, 2025

Choose a reason for hiding this comment

Uh oh!

ratmice Mar 16, 2025

Choose a reason for hiding this comment

Uh oh!

ltratt commented Mar 16, 2025

Uh oh!

ratmice Mar 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ratmice Mar 16, 2025

Choose a reason for hiding this comment

Uh oh!

ratmice Mar 16, 2025

Choose a reason for hiding this comment

Uh oh!

ratmice commented Mar 16, 2025

Uh oh!

ltratt commented Mar 16, 2025

Uh oh!

ratmice commented Mar 16, 2025

Uh oh!

Uh oh!

ratmice commented Mar 16, 2025

Uh oh!

ltratt commented Mar 16, 2025

Uh oh!

ratmice commented Mar 16, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ratmice commented Mar 16, 2025 •

edited

Loading

ratmice Mar 16, 2025 •

edited

Loading

ratmice Mar 16, 2025 •

edited

Loading